Skip to content

assorted bugs: tool timeouts, schema files, and verifyinput callbacks#2511

Merged
sawka merged 3 commits intomainfrom
sawka/fix-tool-timeouts
Nov 4, 2025
Merged

assorted bugs: tool timeouts, schema files, and verifyinput callbacks#2511
sawka merged 3 commits intomainfrom
sawka/fix-tool-timeouts

Conversation

@sawka
Copy link
Member

@sawka sawka commented Nov 4, 2025

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

This PR modifies configuration, frontend state management, and backend tool validation across three areas. The Electron builder config adds unpacking of Monaco editor schema files from ASAR archives. The AIToolUse React component refactors keepalive logic to use persistent refs (partsRef, toolCallIdRef) instead of closures to prevent stale data references in intervals and handlers, with simplified effect dependencies. The Go backend tools (read_dir, read_text_file) gain input verification hooks that validate and expand file paths, enforce directory/file type constraints, and block sensitive files before tool execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • frontend/app/aipanel/aitooluse.tsx: Review the ref usage pattern and effect dependency changes to ensure closures properly capture latest values without unintended re-registrations of intervals
  • pkg/aiusechat/tools_readdir.go & tools_readfile.go: Verify path validation logic handles edge cases and prevent path traversal vulnerabilities; check error handling paths for input verification
  • electron-builder.config.cjs: Confirm ASAR unpacking path pattern is correct for Monaco schema files

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive No description was provided by the author, making it impossible to evaluate whether it relates to the changeset. Add a pull request description explaining the purpose and impact of these changes, even if brief.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: tool timeout fixes (via persistent refs preventing stale closures), schema file unpacking, and verification input callbacks in tool definitions.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/fix-tool-timeouts

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 523cea1 and dc13d87.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • electron-builder.config.cjs (1 hunks)
  • frontend/app/aipanel/aitooluse.tsx (5 hunks)
  • pkg/aiusechat/tools_readdir.go (3 hunks)
  • pkg/aiusechat/tools_readfile.go (2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2430
File: frontend/app/aipanel/aimessage.tsx:137-144
Timestamp: 2025-10-14T06:30:54.763Z
Learning: In `frontend/app/aipanel/aimessage.tsx`, the `AIToolUseGroup` component splits file operation tool calls into separate batches (`fileOpsNeedApproval` and `fileOpsNoApproval`) based on their approval state before passing them to `AIToolUseBatch`. This ensures each batch has homogeneous approval states, making group-level approval handling valid.
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.

Applied to files:

  • pkg/aiusechat/tools_readdir.go
  • pkg/aiusechat/tools_readfile.go
📚 Learning: 2025-10-14T06:30:54.763Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2430
File: frontend/app/aipanel/aimessage.tsx:137-144
Timestamp: 2025-10-14T06:30:54.763Z
Learning: In `frontend/app/aipanel/aimessage.tsx`, the `AIToolUseGroup` component splits file operation tool calls into separate batches (`fileOpsNeedApproval` and `fileOpsNoApproval`) based on their approval state before passing them to `AIToolUseBatch`. This ensures each batch has homogeneous approval states, making group-level approval handling valid.

Applied to files:

  • frontend/app/aipanel/aitooluse.tsx
🧬 Code graph analysis (2)
pkg/aiusechat/tools_readdir.go (2)
pkg/aiusechat/uctypes/usechat-types.go (1)
  • UIMessageDataToolUse (140-151)
pkg/wavebase/wavebase.go (1)
  • ExpandHomeDir (141-155)
pkg/aiusechat/tools_readfile.go (2)
pkg/aiusechat/uctypes/usechat-types.go (1)
  • UIMessageDataToolUse (140-151)
pkg/wavebase/wavebase.go (1)
  • ExpandHomeDir (141-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build for TestDriver.ai
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
🔇 Additional comments (8)
electron-builder.config.cjs (1)

37-41: LGTM!

The addition of Monaco editor schema files to the ASAR unpack list is appropriate. Monaco needs direct filesystem access to these files, and unpacking them ensures they're available at runtime.

frontend/app/aipanel/aitooluse.tsx (3)

84-102: Excellent fix for stale closure in keepalive interval.

The ref pattern correctly ensures the interval always accesses the latest parts array. Without this, the interval would capture stale parts from when the effect first ran, causing keepalive calls to reference outdated tool call IDs if parts changed.

Removing parts from the dependency array is correct since the ref provides access to current values without triggering effect recreation.


236-256: Consistent application of the ref pattern for keepalive stability.

The toolCallIdRef follows the same pattern as partsRef in AIToolUseBatch, ensuring the keepalive interval references the current tool call ID even if toolData.toolcallid changes between renders.


406-416: Clearer batch insertion logic.

The restructured if-else chain ensures each batch type (approval-needed vs. no-approval) is inserted exactly once when the first matching part is encountered, preventing duplicate batch insertions. The explicit flag checks (addedApprovalBatch, addedOtherBatch) make the single-insertion guarantee clear.

pkg/aiusechat/tools_readdir.go (2)

55-76: Well-implemented input verification hook.

The verification function provides early validation before tool execution:

  1. Parses and validates input parameters
  2. Expands home directory paths
  3. Confirms the path exists via os.Stat
  4. Enforces that the path is a directory

The directory check at lines 71-73 prevents tool misuse by guiding users to use read_text_file for files instead.

Note: There's a small TOCTOU window between verification and execution where the path could be deleted or changed, but this is acceptable for this use case.


157-157: Verification hook properly integrated.

The ToolVerifyInput assignment ensures inputs are validated before the tool callback executes, providing better error messages and preventing unnecessary work.

pkg/aiusechat/tools_readfile.go (2)

200-225: Robust input verification with security checks.

The verification function implements defense-in-depth validation:

  1. Input parsing and validation
  2. Path expansion with home directory support
  3. Security check: Blocks access to sensitive files (credentials, keys, etc.) early
  4. Path existence verification
  5. Type check to prevent directory reads

The early sensitive file check at lines 211-213 is particularly valuable—it prevents unnecessary I/O and provides immediate feedback when users attempt to access blocked files.

Note: As with verifyReadDirInput, there's a minor TOCTOU window between verification and execution, which is acceptable here.


400-400: Verification hook properly integrated.

Consistent with the pattern in tools_readdir.go, the ToolVerifyInput assignment enables early validation and better error reporting.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters
The command is terminated due to an error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sawka sawka merged commit dfa34c1 into main Nov 4, 2025
8 checks passed
@sawka sawka deleted the sawka/fix-tool-timeouts branch November 4, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant